Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix record update attempt with invalid IP #203

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

irfanhakim-as
Copy link

@irfanhakim-as irfanhakim-as commented Nov 26, 2024

Issues addressed

Changes

This PR is a two-parter:

Part 1

  • This is a safe change and non-disruptive in the sense that it does not change any existing behaviour other than the fact that it ensures when the endpoint that is being used to determine the public IPv4/6 address is unavailable or not returning the intended address correctly, it fails instead of returning the error response as the supposed IP address (which causes unwanted, repetitive attempts to update DNS records with this error response).

  • It fixes the linked issue only partially - it fixes the unintended attempts of updating the DNS records with error response values it thought was the IPv4/6 address, but it still is affected by downtimes or currently broken (primary/fallback) Cloudflare endpoints (https://1.1.1.1/cdn-cgi/trace and https://1.0.0.1/cdn-cgi/trace).

  • Commits: f0d9510...cbb5ead

  • Test image: ghcr.io/irfanhakim-as/cloudflare-ddns:1.0.2-fix-wrong-ip-r1

Part 2

  • Includes Part 1, is safe, but adds a few more modifications that changes an existing behaviour slightly.

  • It fixes the linked issue completely - by adding support for acquiring IPv4/6 address from single-value endpoints (i.e. https://ipv6.icanhazip.com) in addition to key-value endpoints as before (i.e. https://[2606:4700:4700::1111]/cdn-cgi/trace).

  • This includes adding 2 new global variables (ipv4_endpoints and ipv6_endpoints), which are used to specify the primary and fallback endpoints for both IP types. These global variables could potentially be used as new config options (by supplying 2-item list of primary and fallback endpoints for each IP type) in the future.

  • The previous default fallback endpoints were updated from https://1.0.0.1/cdn-cgi/trace and https://[2606:4700:4700::1001]/cdn-cgi/trace to https://ipv4.icanhazip.com and https://ipv6.icanhazip.com respectively.

  • Commits: f0d9510...cbb5ead and cbb5ead...ba37a97

  • Test image: ghcr.io/irfanhakim-as/cloudflare-ddns:1.0.2-fix-wrong-ip-r2

Notes

  • I'm not sure about the "usual" or "proper" way of contributing to this repo, i.e. if I need to update the version number in the main script, but feel free to let me know if there's anything I can do to smoothen this PR in case you intend to merge it!
  • My test images were only built for linux/amd64 for simplicity - if you are testing this on a different platform, please try the following image instead which I have built for all 7 platforms the official image supports: ghcr.io/irfanhakim-as/cloudflare-ddns:1.0.2-fix-wrong-ip-r3

Reduces code repetition and ensures failures get raised
Allows acquiring IPv4/6 address from key-value (i.e. https://1.1.1.1/cdn-cgi/trace) and single-value (i.e. https://ipv4.icanhazip.com)
These vars could potentially be added as new config options to set the primary and fallback endpoints
Sensible fallback endpoint in case Cloudflare's are unavailable
Copy link

@KhooHaoYit KhooHaoYit left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have service disruption twice because it wasn't updating my domain ip but thanks to your PR I could temporary fix the issue by mounting the file and override it in the container

Thanks man, I appeciate your PR

@madereddy
Copy link

Had the same issue where underlying IP rotated and it failed to update the IPs. This fixed my issue as well.

@AverageHelper
Copy link

AverageHelper commented Dec 3, 2024

Here for the same outage as well. Thanks for the PR!!

@makutaku
Copy link

makutaku commented Dec 3, 2024

Shouldn't this be handled like a transaction, meaning, it should not clear existing A record unless it can successfully update it ?

Copy link

@DarrenOfficial DarrenOfficial left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works like a charm

@orelgs
Copy link

orelgs commented Jan 21, 2025

How can I get this fix?
I tried updating my stack to use this image: ghcr.io/irfanhakim-as/cloudflare-ddns:1.0.2-fix-wrong-ip-r2
But the container logs are just:
exec /usr/local/bin/python: exec format error

@irfanhakim-as
Copy link
Author

How can I get this fix? I tried updating my stack to use this image: ghcr.io/irfanhakim-as/cloudflare-ddns:1.0.2-fix-wrong-ip-r2 But the container logs are just: exec /usr/local/bin/python: exec format error

I'm not sure why you're getting that, it does seem that you're pulling the right image ghcr.io/irfanhakim-as/cloudflare-ddns:1.0.2-fix-wrong-ip-r2.

Do note that while I did find some things in the (source) script that could use some improvement, this PR of mine aims to only fix the things I've mentioned in my description of the PR - as I try to make the PR as least disruptive as possible.

That being said, I've not seen the error you've mentioned deploying the exact same image for the past few months using my Helm chart.

@orelgs
Copy link

orelgs commented Jan 22, 2025 via email

@irfanhakim-as
Copy link
Author

irfanhakim-as commented Jan 22, 2025

Any chance you could upload it to docker or something?I basically just replaced the "image" section in my already existing docker compose file for cloudflare-ddnsThat's how I got the error

I don't really use Docker/Dockerhub anymore, though it shouldn't matter if I upload there - what matters is if you're able to actually pull the image and are currently using it in your docker-compose.

Can you try pulling the image on your system using docker:

docker pull ghcr.io/irfanhakim-as/cloudflare-ddns:1.0.2-fix-wrong-ip-r2

If it's downloaded successfully, list down your container images and check its IMAGE ID value:

docker images | grep ghcr.io/irfanhakim-as/cloudflare-ddns | awk '{print $3}'

Its ID should be:

5d1f94290180

If you got the correct image/ID, and your docker-compose is also specifying/using the exact same image, then I'm not sure why you got that error other than perhaps a configuration issue of sort in your deployment. You can share your setup for us to check, including docker-compose manifest or any configurations, but if you do be sure to redact any sensitive information beforehand.

Though, my expectation is, if you get that error with my test image (ghcr.io/irfanhakim-as/cloudflare-ddns:1.0.2-fix-wrong-ip-r2), you should also be getting the exact same error with the "official" image (ghcr.io/timothymiller/cloudflare-ddns/cloudflare-ddns@sha256:ded91de91844e26feb7dbe7f902d754e77857514f1a75fd3a18a9ca1a6722a74).

@orelgs
Copy link

orelgs commented Jan 22, 2025 via email

@makutaku
Copy link

The master is unusable.
When will this change be merged to master ?

@irfanhakim-as
Copy link
Author

Unfortunately, I don't think anyone with write perms to this repo has noticed/reviewed this PR just yet. There hasn't been a single commit pushed to Master since 6 months ago.

cc: @timothymiller

@makutaku
Copy link

makutaku commented Jan 31, 2025

If the repo seems abandoned, I'd support your fork.

@QbikEdge
Copy link

QbikEdge commented Feb 1, 2025

How can I get this fix? I tried updating my stack to use this image: ghcr.io/irfanhakim-as/cloudflare-ddns:1.0.2-fix-wrong-ip-r2 But the container logs are just: exec /usr/local/bin/python: exec format error

Same for me. I also can not run the ghcr.io/irfanhakim-as/cloudflare-ddns:1.0.2-fix-wrong-ip-r2.
And im getting the same error.

My Compose file looks like this:

version: '3.9'
services:
  cloudflare-ddns:
    # image: timothyjmiller/cloudflare-ddns:latest
    image: ghcr.io/irfanhakim-as/cloudflare-ddns:1.0.2-fix-wrong-ip-r2
    container_name: cloudflare-ddns
    security_opt:
      - no-new-privileges:true
    network_mode: 'host'
    environment:
      - PUID=1000
      - PGID=1000
    volumes:
      - /cloudflare-config.json:/config.json
    restart: unless-stopped

@irfanhakim-as
Copy link
Author

irfanhakim-as commented Feb 2, 2025

I'm not entirely sure, but the issue might have to do with this:

    network_mode: 'host'

and your host system might be incompatible with my image that is built for amd64 (x86_64) architecture rather than ARM.

Again, I could be wrong as I almost never host my deployments using Docker directly (only through Kubernetes), and I especially never host them with host privileges to ensure as reproducible and isolated of a deployment environment as possible.

I suspect though that it might be an architecture incompatibility, as my PR definitely have not changed anything that might cause this issue. I only built this image for amd64 for testing purposes, compared to the official image which builds and releases their image for multiple platforms (including ARM). This difference might explain why this issue might not occur on the official image.

Assuming those having this issue is hosting this service on a non linux/amd64 platform (specifically arm64), I've deployed another image that should support (I've never done this before, so bear with me) both linux/amd64 and linux/arm64/v8. Please try it out:

If it still does not work, please share your architecture from the machine you are hosting the deployment:

uname -m

cc: @QbikEdge @orelgs

@irfanhakim-as
Copy link
Author

Assuming those having this issue is hosting this service on a non linux/amd64 platform (specifically arm64), I've deployed another image that should support (I've never done this before, so bear with me) both linux/amd64 and linux/arm64/v8.

Instead of supporting only linux/amd64 and linux/arm64, I've rebuilt the image under the same ghcr.io/irfanhakim-as/cloudflare-ddns:1.0.2-fix-wrong-ip-r3 tag to now support all 7 platforms the official image supports:

  • linux/ppc64le
  • linux/s390x
  • linux/386
  • linux/arm/v6
  • linux/arm/v7
  • linux/arm64/v8
  • linux/amd64

Going forward, please test this image instead and see if it works.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants